CAMEL-24367: Add camel-rest-postman component - #25390
Conversation
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for this well-designed component, @christosgkoros — the security measures (redirect rejection, HTTPS enforcement, bounded reads, auth stripping, variable recursion limits) are all substantiated by the code, the test suite is thorough (122 test methods, all AssertJ, all package-private, no Thread.sleep), and there are no new runtime dependencies.
Two things need addressing before this can merge, plus a few suggestions below.
Blocking
-
Missing JIRA ticket — no
CAMEL-XXXXXissue is linked anywhere (PR title, description, commits, branch name). Per project guidelines, a JIRA ticket is required. Please create one and update the branch/commits accordingly (feature/CAMEL-XXXXX-rest-postman,CAMEL-XXXXX: Add camel-rest-postman component). -
Upgrade guide misuse — the 37-line new-component section added to
camel-4x-upgrade-guide-4_22.adocshould be removed. Per project conventions, the upgrade guide is for migration only — new features should not be documented there. The component's own.adocpage (which is well-written) is the right place.
Design note
Reusing RestOpenApiConsumerFactory — this is fine, no need for a new SPI. The contract is generic enough and PlatformHttpComponent is its only implementation. Just be aware the parameter description for consumerComponentName says "RestOpenApiConsumerFactory" which may confuse users — consider describing the capability generically instead of naming the SPI class.
This review covers project rules and conventions. It does not replace specialised tools (CodeRabbit, SonarCloud) for deep static analysis.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
73a054b to
c348957
Compare
christosgkoros
changed the title
|
Thanks for the review @davsclaus, and for creating CAMEL-24367. All four points are addressed in the force-pushed commit 1. JIRA — the commit message and PR title are now On the branch name: I tried renaming it to 2. Upgrade guide — the 37-line section is removed. One observation while doing this, purely FYI: the same file currently has 3.
4. Thanks also for confirming the Rebuilt and re-verified after the changes: 167 tests in Claude Code on behalf of @christosgkoros |
|
Ad 2) |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
github-actions
Bot
added
components
core
catalog
docs
dsl
core-build-and-dependencies
labels
|
[05:56:02.037] WARN (asciidoctor): skipping reference to missing attribute: petid |
|
The The site build produced exactly four asciidoctor warnings and all four were in AsciiDoc was parsing the brace spans as attribute references rather than literal text. They are now wrapped in an inline passthrough ( Note that the same job log also contains I have left this as a separate commit so the change since your review is visible; it should be squashed at merge. Claude Code on behalf of @christosgkoros |
|
There are uncommitted changes |
|
Fixed in
To make sure this is actually fixed rather than just locally plausible, I reproduced what CI does end to end: Also confirmed on the previous run that this was the only real failure: on While I was at it I checked that Claude Code on behalf of @christosgkoros |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
PR #25390 Review: CAMEL-24367 — Add camel-rest-postman component
Nice work on this — the component is well-structured and the security posture around the Postman API key is notably thorough (redirect prevention, HTTPS enforcement, bounded reads, auth redaction, SHA-256 cache keys). Two things worth considering:
Findings
1. Variable resolver allows Camel property expansion from collection content (Medium — security documentation)
PostmanVariableResolver.lookup()falls back tocamelContext.resolvePropertyPlaceholders("{{" + name + "}}")wherenameoriginates from collection content. If a cloud-sourced collection (editable by anyone with Postman workspace access) contains{{env:AWS_SECRET_ACCESS_KEY}}or{{sys:user.home}}, Camel's resolver would expand those, and the values could appear in outbound HTTP request URLs or headers.- While the route author controls the collection source, this is a trust-boundary concern. Consider either (a) documenting this risk in the component adoc, or (b) restricting the fallback to names that don't contain
:(which would blockenv:,sys:,bean:prefixes).
2. Per-call HttpClient creation in PostmanCloudClient.fetchCollection() (Low)
- Each call creates a new
java.net.http.HttpClient, which allocates an internal thread pool. Under repeated cache misses this wastes OS threads. The client could be built once and reused.
Questions
- Sub-exchange lifecycle in
RestPostmanRunnerProducer.process()— Sub-exchanges created inside the runner loop are never explicitly released viareleaseUnitOfWork(). Under heavy concurrent use this relies on GC rather than deterministic cleanup. Is this intentional?
Positive observations
- Clean Camel component conventions:
DefaultComponent/DefaultEndpoint, proper annotations,SSLContextParametersAware, no Lombok, records only for internals - Security is unusually solid: redirect prevention, HTTPS enforcement, bounded reads (8 MiB / 5000 items / 64 depth), auth redaction, secret variable scrubbing, event scripts never parsed
- No new third-party dependencies — entire parser uses
camel-util-json - Strong test coverage: 14 test files, 167+ tests, WireMock for cloud API, consumer tests in
camel-platform-http-vertx - Commit conventions followed (
CAMEL-24367: ...), JIRA linked, AI assistance disclosed SensitiveUtils.javachange is auto-generated — standard for new secret options
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 572 tested, 23 compile-only — current: 565 all testedMaveniverse Scalpel detected 595 affected modules (current approach: 565).
|
Code review — PR #25390 (CAMEL-24367:
|
| Area | Status |
|---|---|
| Design & architecture | ✅ Excellent |
| Security | ✅ Strong (document property-expansion edge case) |
| Tests | ✅ Thorough |
| Docs | ✅ Good (AsciiDoc fix applied) |
| Conventions | ✅ Matches AGENTS.md |
| Maintainer blockers | ✅ Addressed |
| Rebase hygiene |
Request changes → re-request review after rebase onto latest main. Happy to re-review once the upgrade guide diff is clean.
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Request changes
Please address the following before merge:
Required
1. Rebase onto latest main — upgrade guide drift
The branch is behind current main. Comparing camel-4x-upgrade-guide-4_22.adoc shows this PR would drop unrelated upgrade-guide sections already on main (e.g. camel-mcp-server session eviction, camel-support deserialization filter notes).
Please rebase onto latest main and confirm the upgrade guide file has zero diff against main (new components belong only in the component's own .adoc page, not the upgrade guide).
Everything else from the prior review looks good: security design, test coverage, maintainer feedback (JIRA, exception wrapping, docs, derived file regen), and CI are all in solid shape.
Once rebased, please re-request review.
AI-generated review on behalf of @atiaomar1978-hub
Configures REST producers and contract-first REST consumers from a Postman Collection, as the Postman counterpart of camel-rest-openapi. Like it, this component performs no HTTP itself and delegates to a component implementing RestProducerFactory. The collection is loaded either from a Collection v2.1 JSON document (classpath:, file: or http:) or, by its uid, from the Postman cloud. Producer: - a fragment naming a request invokes it, sending the exchange body and headers - a fragment naming a folder, or no fragment at all, runs every request in turn like Postman's collection runner and returns a List<PostmanRunResult> Consumer: - serves the collection's requests, dispatching each to direct:<requestId> - missingRequest=fail|ignore|mock, where mock replays the collection's own saved example responses before falling back to mockIncludePattern - apiContextPath serves the collection with every auth block and every secret variable removed Requests are addressed by their slugified name, folder-qualified when a name is not unique, and by item.id when the collection records one. item.id is optional in the v2.1 schema and Postman's exporter strips it, so exported collections are normally addressed by slug. Two separate credentials are kept apart by their option names: postmanApiKey authenticates against Postman in order to download a collection and is never sent to the API the collection describes, while the collection's own auth block is governed by collectionAuth, which defaults to ignore. Redirects from postmanApiUrl are rejected rather than followed, since following one would send the key to the redirect target. No new third-party dependency is introduced: the collection is parsed with camel-util-json. Co-Authored-By: Claude <noreply@anthropic.com>
The documentation build treats asciidoctor warnings as failures, and the four brace spans in rest-postman-component.adoc were being parsed as AsciiDoc attribute references rather than literal text, producing "skipping reference to missing attribute" for petid, baseurl, variable and placeholders. Wrap them in an inline passthrough so no substitution is applied. The doubled Postman braces make this preferable to backslash escaping. Co-Authored-By: Claude <noreply@anthropic.com>
The consumerComponentName description reword and the AsciiDoc passthrough fix were not propagated to the files derived from them, which left the tree dirty after a build: - catalog copy of rest-postman-component.adoc - component and endpoint DSL builder factories Also reverts an unintended re-indentation of the SENSITIVE-PATTERN marker in SensitiveUtils, which the formatter had applied but the generator does not produce, so that file now differs from main only by the two postmanapikey entries. Co-Authored-By: Claude <noreply@anthropic.com>
Do not resolve Camel property placeholder functions from collection content.
PostmanVariableResolver fell back to resolvePropertyPlaceholders for any name
it did not know, so a collection containing {{env:AWS_SECRET_ACCESS_KEY}} or
{{sys:user.home}} would have that value expanded into an outgoing request. A
cloud-hosted collection is editable by anyone with access to the Postman
workspace, so names using the prefix:value syntax of a placeholder function
are now refused. Plain names still resolve, so an operator can override any
variable through properties.
Build the HttpClient once in PostmanCloudClient rather than per fetch. Each
client allocates its own selector and executor threads, which were leaked
across repeated cache misses.
Release the unit of work of the sub-exchanges created by the collection
runner. Nothing in the endpoint-to-producer path starts one today, so this is
defensive, but it keeps cleanup deterministic rather than depending on that
invariant holding.
Co-Authored-By: Claude <noreply@anthropic.com>
d4c5f8e to
70a80ea
Compare
|
@davsclaus thanks — all three addressed in 1. Variable resolver expanding Camel property functions (Medium) — good catch, and I took option (b) rather than only documenting it, since documentation would not stop it.
Three tests were added: that 2. Per-call 3. Sub-exchange lifecycle — not intentional so much as unexamined, so thank you for asking. Nothing in the Verified after the change: 170 tests in Claude Code on behalf of @christosgkoros |
|
@atiaomar1978-hub rebased onto latest One clarification on the finding itself, for the record rather than to quibble: this PR never modified that file, so a merge would not have dropped the The section was added in the first push and removed in the amend that addressed @davsclaus's review, so no commit on the branch touches the file. The 193-line difference you saw was the branch being stale relative to Post-rebase verification: full build plus the codegen chain leaves the tree clean, 170 tests in Worth noting one thing the rebase surfaced: regenerating the catalog after pulling in Re-requesting review. Claude Code on behalf of @christosgkoros |
Updated review (post
|
| Item | Status |
|---|---|
Upgrade guide drift vs main |
Fixed — camel-4x-upgrade-guide-4_22.adoc has zero diff against apache/camel main |
| JIRA, exception wrapping, docs passthrough, derived file regen | Fixed (per @davsclaus feedback) |
PostmanVariableResolver placeholder-function injection ({{env:...}}, {{sys:...}}, {{bean:...}}) |
Fixed in 70a80ea — isSafeToResolveAsProperty() blocks prefix:value names; covered by tests |
Per-fetch HttpClient thread leak in PostmanCloudClient |
Fixed — client built once in constructor |
Sub-exchange UoW leak in RestPostmanRunnerProducer |
Fixed — releaseUnitOfWork(sub) in runner loop |
Remaining (non-blocking)
- 405 / empty
Allowon matched paths — author-documented gap when vert.x router handles wrong-method before the processor runs. Acceptable for v1; optional follow-up withplatform-http. - Branch name
feat/camel-rest-postman-componentvsfeature/CAMEL-24367-...— convention only.
Highlights
- Strong security design: separated Postman API key vs collection auth, redirect rejection, HTTPS enforcement, bounded reads, auth/secret redaction, no script execution
- No new third-party dependencies
- 167 component tests + 8
platform-httpconsumer tests - Clear component documentation
Ready for committer merge review.
AI-generated review on behalf of atiaomar1978-hub
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Prior request-changes items (upgrade guide rebase, placeholder-function security, HttpClient reuse, UoW cleanup) are addressed in 70a80ea. Approve for committer merge review.
atiaomar1978-hub
left a comment
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
All prior feedback addressed in 70a80ea. Approve for merge.
|
LGTM but we need to get 4.22.0 released first and then bump maven for 4.23 release, and after that this can be rebased and then adjusted to be 4.23 as well. |
Thanks, I understand this PR was submitted right as the team was preparing for 4.22. I will update it when 4.23 hits |
3 participants
JIRA: https://issues.apache.org/jira/browse/CAMEL-24367
What this adds
A new
camel-rest-postmancomponent that configures REST producers and contract-first REST consumers from a Postman Collection instead of an OpenAPI specification. It is the Postman counterpart ofcamel-rest-openapi: it performs no HTTP itself and delegates to a component implementingRestProducerFactory.The motivation is that a large number of teams keep a Postman Collection as the only machine-readable description of their API, and today Camel has no way to consume that.
The collection is loaded either from a Collection v2.1 JSON document (
classpath:,file:,http:) or, by its uid, from the Postman cloud.Usage
Multi-request runs return a
List<PostmanRunResult>(status, body, headers, per-request failure), withrunFailFastcontrolling whether the first failure aborts the run.Design notes
Addressing requests. Postman items have a human name rather than an operation id, so the name is slugified (
Get Pet By Id→getPetById), folder-qualified (pets/getPetById) when a name is not unique.item.idis accepted too, but note it is optional in the v2.1 schema and Postman's exporter strips it, so exported collections are normally addressed by slug and cloud-fetched ones by id. Both work.Two credentials, deliberately named apart.
postmanApiKeyauthenticates against Postman in order to download a collection; it is never sent to the API the collection describes. The collection's ownauthblock authenticates against that API and is governed bycollectionAuth, which defaults toignore(with a startup warning naming the type found) because those values are usually unresolved{{placeholders}}, and silently attaching a credential found in a config file to outbound requests is surprising. An e2e test asserts the separation.Security. Redirects from
postmanApiUrlare rejected rather than followed, since following one would replay the API key to the redirect target;postmanApiUrlmust be HTTPS except for loopback; reads are bounded (8 MiB, 5000 items, 64 folder levels);apiContextPathserves the collection with everyauthblock and everytype: secretvariable removed, unconditionally. Postmaneventscripts are never parsed or executed.No new third-party dependency. The collection is parsed with
camel-util-json, already on the classpath viacamel-support.Testing
camel-rest-postmancamel-platform-http-vertx, following the precedent thatrest-openapi's consumer tests live there becausePlatformHttpComponentis the onlyRestOpenApiConsumerFactoryimplementationmvn clean install -Psourcecheckpasses on both modified modulesReview feedback addressed
Changes since the first push, in response to @davsclaus's review:
camel-4x-upgrade-guide-4_22.adocis now byte-identical tomain. The component's own.adocpage carries that documentation.consumerComponentNamedescription — no longer names the OpenAPI SPI class; it now describes the capability ("must be able to service contract-first REST consumers, as platform-http does").new RuntimeException(e)sites inRestPostmanProcessornow useRuntimeCamelException.wrapRuntimeCamelException(e).Known gap
For a path the collection does describe, a wrong-method request currently gets a 405 from the vert.x router before this component's processor runs, and the router leaves
Allowempty.rest-openapipopulatesAllowin the equivalent case, so the difference is mine; the processor's own 404/405 handling (withAllow) still applies to paths the router has no route for. I would appreciate a pointer here if the cause is obvious to someone who knowsplatform-httpwell.This contribution was AI-assisted: written with Claude Code (Claude Opus) on behalf of @christosgkoros, who reviewed the design decisions. Commits carry a
Co-Authored-Bytrailer.